-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TDL-15318: PK for track_events stream #50
base: master
Are you sure you want to change the base?
Conversation
tests/tap_tester/base.py
Outdated
self.REPLICATION_KEYS: {'browser_time'} | ||
}, | ||
"track_events": { | ||
self.PRIMARY_KEYS: {"track_type_id", "visitor_id", "account_id", "server", "remote_ip", "user_agent", "day"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this can we write:
"track_type_id", "visitor_id", "account_id", "server", "remote_ip", "user_agent", "day" if self.is_day_range else "hour"}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tests/tap_tester/base.py
Outdated
"events": { | ||
self.PRIMARY_KEYS: {"visitor_id", "account_id", "server", "remote_ip"}, | ||
self.REPLICATION_METHOD: self.INCREMENTAL, | ||
self.REPLICATION_KEYS: {'day'} if self.is_day_range else {'hour'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of putting dict in if-else, we can put if-else inside dict for string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
CircleCI build is failing currently as we have added a standard discover test to verify updated primary key but replication keys are not with expected inclusion currently. The replication key with valid inclusion is fixed as part of #45. |
Blocked on merging this PR since there is a discussion going on in the JIRA (https://jira.talendforge.org/browse/TDL-15318) regarding the correct Primary Key to choose for this stream. |
* TDL-15317: Updated primary key for feature-events * Moved integration test * TDL-15317: Updated integration test * updated readme file * Resolved internal PR review comments * Resolved review comments * run bookmark test with hour and day range Co-authored-by: harshpatel4_crest <[email protected]> Co-authored-by: Harsh <[email protected]>
Description of change
TDL-15318: PK for track_events stream
The updated primary key with a combination of the below fields:
Reference: https://developers.pendo.io/docs/?bash#events-grouped
Manual QA steps
Risks
Rollback steps